use owner refs to determine ownership for ASO resources #4499
Conversation
k8s-ci-robot
added
release-note
| // | ||
| // Deprecated: OwnerReferences now determine ownership. |
There was a problem hiding this comment.
This syntax makes sure the linter flags any additional usages of this constant.
| // ResourceRef returns a concrete, named ASO resource type to facilitate a | ||
| // strongly-typed GET. Namespace is not read if set here and is instead | ||
| // derived from OwnerReferences. |
There was a problem hiding this comment.
OwnerReferences can't span namespaces, so this is why namespace is being removed in lots of places throughout this PR.
| if !ownedByCluster(existing.GetLabels(), r.clusterName) { | ||
| if isOwned, err := isOwnedBy(resource, r.owner, r.Scheme()); err != nil { | ||
| return zero, err | ||
| } else if !isOwned && !hasLegacyOwnedByLabel(resource.GetLabels(), r.clusterName) { |
There was a problem hiding this comment.
This line should fulfill back-compat.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4499 +/- ##
==========================================
+ Coverage 62.11% 62.16% +0.05%
==========================================
Files 189 189
Lines 18639 18633 -6
==========================================
+ Hits 11577 11584 +7
+ Misses 6425 6410 -15
- Partials 637 639 +2 ☔ View full report in Codecov by Sentry. |
| if err := controllerutil.SetControllerReference(r.owner, parameters, r.Client.Scheme()); err != nil { | ||
| return zero, errors.Wrap(err, "failed to set owner ref") | ||
| } |
There was a problem hiding this comment.
The reason we're not adding the owner ref only on create like were adding the label before is because we want to set the owner ref also on existing resources during a CAPZ upgrade.
|
This lgtm. Do we have any additional BYO E2E scenarios we can light up to validate the various flavors of owner-ness? |
|
The optional tests do some BYO things. Otherwise we're tracking fleshing out these kinds of tests more in #3545. /test pull-cluster-api-provider-azure-e2e-optional |
| @@ -422,6 +422,7 @@ func (s *ManagedControlPlaneScope) IsVnetManaged() bool { | |||
| defer done() | |||
|
|
|||
| vnet := s.VNetSpec().ResourceRef() | |||
| vnet.SetNamespace(s.ASOOwner().GetNamespace()) | |||
There was a problem hiding this comment.
Why is the namespace set here explicitly? I see that other resources have the namespace set in the generic aso resource, but I could be misunderstanding how that works.
There was a problem hiding this comment.
The comment on the interface definition for ResourceRef() now explains that it no longer reads the namespace from there and instead uses the namespace of the owner. This is to account for that change. On the plus side, we won't have to add a namespace field to the Spec types for all the services we have yet to convert to ASO.
| Namespace: namespace, | ||
| Labels: map[string]string{ | ||
| infrav1.OwnedByClusterLabelKey: "not-" + clusterName, | ||
| }, |
There was a problem hiding this comment.
Should we keep a test case that uses the legacy label to test back compat?
There was a problem hiding this comment.
That scenario is already tested in the aso package, so I'm not sure if it's worth duplicating that here.
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 71306c7fbc6ba18e49c581d8c1a77363fe2708d7
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/lgtm |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR makes CAPZ add an OwnerReference to each ASO resource it controls (i.e. any ASO resource not pre-created by a user). This is used instead of the
sigs.k8s.io_cluster-api-provider-azure_ownedlabel that's currently used. This better enables #4338 by tracking ownership more granularly than only by Cluster (such as per-AzureMachine or per-AzureManagedMachinePool) and #4339. More motivation is spelled out in #4340.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #4340
Special notes for your reviewer:
TODOs:
Release note: